[BranchRelaxation] Fix invalid branch generation in branch-relaxation #162065
Conversation
|
@llvm/pr-subscribers-backend-risc-v Author: None (sc-clulzze) ChangesIf we have MBB with only one successor which is accessable through both conditional and unconditional branches (TBB == FBB), in Fixes: #162063 Full diff: https://github.com/llvm/llvm-project/pull/162065.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/BranchRelaxation.cpp b/llvm/lib/CodeGen/BranchRelaxation.cpp
index 2d50167faa085..bb75c638d1bcc 100644
--- a/llvm/lib/CodeGen/BranchRelaxation.cpp
+++ b/llvm/lib/CodeGen/BranchRelaxation.cpp
@@ -491,6 +491,20 @@ bool BranchRelaxation::fixupConditionalBranch(MachineInstr &MI) {
return true;
}
if (FBB) {
+ // If we get here with a MBB which ends like this:
+ //
+ // bb.1:
+ // successors: %bb.2;
+ // ...
+ // BNE $x1, $x0, %bb.2
+ // PseudoBR %bb.2
+ //
+ // Just remove conditional branch.
+ if (TBB == FBB) {
+ BlockInfo[MBB->getNumber()].Size -= TII->getInstSizeInBytes(MI);
+ MI.eraseFromParent();
+ return true;
+ }
// We need to split the basic block here to obtain two long-range
// unconditional branches.
NewBB = createNewBlockAfter(*MBB);
diff --git a/llvm/test/CodeGen/RISCV/branch-rel.mir b/llvm/test/CodeGen/RISCV/branch-rel.mir
new file mode 100644
index 0000000000000..218ebe1d09f44
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/branch-rel.mir
@@ -0,0 +1,24 @@
+# NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+# RUN: llc %s -mtriple=riscv64 -run-pass=branch-relaxation -o - -verify-machineinstrs | FileCheck %s
+
+--- |
+ define void @foo() {
+ ret void
+ }
+...
+---
+name: foo
+tracksRegLiveness: true
+body: |
+ bb.0:
+ liveins: $x1
+ BNE $x1, $x0, %bb.3
+ PseudoBR %bb.3
+ bb.1:
+ liveins: $x1
+ INLINEASM &".space 4096", 1
+ BGE $x1, $x0, %bb.3
+ bb.3:
+ PseudoRET
+## NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
+# CHECK: {{.*}}
|
|
@preames, Could you please take a look? |
| @@ -0,0 +1,24 @@ | |||
| # NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5 | |||
There was a problem hiding this comment.
iirc for a MIR test you need update_mir_test_checks.py not the llc version (which expects assembly output) - this is why there are no real CHECK lines in this output.
There was a problem hiding this comment.
Thanks, fixed it, now there are actual CHECKS.
| if (TBB == FBB) { | ||
| BlockInfo[MBB->getNumber()].Size -= TII->getInstSizeInBytes(MI); | ||
| MI.eraseFromParent(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
For RISC-V, this seems correct.
I'm not entirely sure if we should be using a sequence of removeBranch(MBB); insertUncondBranch(MBB, TBB) instead of just removing one instruction, not being familiar with how all the targets define removeBranch and insertUnconditionalBranch. From the few I reviewed, they mostly do seem straightforward, but that's not guaranteed?
There was a problem hiding this comment.
I am also not entirely sure about all the targets, but looking at functions description inside TargetInstrInfo.h seems like these functions are indeed pretty straightforward. Lets this sequence instead of explicit eraseFromParent like in other parts of this pass.
4 participants
If we have MBB with only one successor which is accessable through both conditional and unconditional branches (TBB == FBB), in
fixupConditionalBranchwe will first replace FBB with NewMBB in successors list -MBB->replaceSuccessor(FBB, NewBB);, and then create branch to TBB -insertBranch(MBB, &NextBB, TBB, Cond);, ending up with two branches to different blocks, but only one successor.Fixes: #162063